Mina-signer: in-SNARK verifiable Signatures #710

mitschabaude · 2023-01-30T20:44:09Z

this is about letting users sign data with mina-signer (i.e., with their wallet), such that the signature can be verified by a smart contract. in other words, we need to make sure there is a signing function in mina-signer whose signatures can be verified in-snark by snarkyjs.

adds signFields and verifyFields to mina-signer
modifies the existing Signature.create and Signature.verify in snarkyjs, to be compatible with signFields
- note that Signature.create is not for in-snark usage, but Signature.verify is

Making snarkyjs signatures compatible uncovered a bug/irregularity in the previous implementation:

after hashing the message to be signed, the algorithm reinterprets the hash (a base field element) as a scalar
in snarkyjs, this was done with Scalar.fromBits(hash.toBits())
however, it turned out that Scalar uses a "shifted" representation and fromBits just interprets the bits to already be in shifted form
so, it can be said that the existing algorithm used a non-standard way of moving from base field to scalar
this is fixed here, by unshifting the scalar. in the case of the in-snark verify, we do the unshifting on the group element level, leveraging the existing Group.scale method. it's not clear to me if there's a more efficient way to do this

mitschabaude

some comments to help with review:

mitschabaude · 2023-01-31T13:30:36Z

src/lib/account_update.ts

@@ -1877,7 +1877,11 @@ function signJsonTransaction(
      accountUpdate.authorization.proof === null
    ) {
      zkappCommand = JSON.parse(
-        Ledger.signAccountUpdate(JSON.stringify(zkappCommand), privateKey, i)
+        Ledger.signOtherAccountUpdate(


unrelated fix

mitschabaude · 2023-01-31T13:30:51Z

src/lib/core.ts

@@ -72,3 +74,8 @@ That means it can't be called in a @method or similar environment, and there's n
    highBit: Bool(x >> lowBitSize === 1n),
  };
 };
+
+Scalar.fromBigInt = function (scalar: bigint) {


missing helper which was useful here

same question as elsewhere, I'm curious how a dev would know not to use these functions if they want them to create constraints? My guess is that we're warning them that going to bigint or from bigint is not constraining anything?

For methods like these, which take a JS value and produce constant field elements, I think no warning is needed -- it's a valid and useful way to introduce constants into the circuit.

Going the other way -- from field elements to JS value -- is a very common mistake, but throws an error as soon as you try to compile the contract. That's because extracting values, under the hood, always works like this:

match x with | Constant x -> x | x -> As_prover.read_var x

so during compile time you'll hit an error for the As_prover.read_var x case. The constant case is fine - no need to prove how constants are computed, since the prover can't change them

mitschabaude · 2023-01-31T13:31:45Z

src/lib/signature.ts

@@ -192,12 +198,23 @@ class Signature extends CircuitValue {
  static create(privKey: PrivateKey, msg: Field[]): Signature {
    const publicKey = PublicKey.fromPrivateKey(privKey).toGroup();
    const d = privKey.s;
-    const kPrime = Scalar.random();
+    const kPrime = Scalar.fromBigInt(
+      deriveNonce(


crypto change: instead of a random nonce, we now use the same deterministic nonce derivation as in mina-signer (and elsewhere in our protocol)

mitschabaude · 2023-01-31T13:33:27Z

src/lib/signature.ts

    let { x: r, y: ry } = Group.generator.scale(kPrime);
    const k = ry.toBits()[0].toBoolean() ? kPrime.neg() : kPrime;
-    const e = Scalar.fromBits(
-      Poseidon.hash(msg.concat([publicKey.x, publicKey.y, r])).toBits()
+    let h = hashWithPrefix(


crypto change: instead of plain hashing with Poseidon, we use a hash prefix

mitschabaude · 2023-01-31T13:34:24Z

src/lib/signature.ts

-    const e = Scalar.fromBits(
-      Poseidon.hash(msg.concat([publicKey.x, publicKey.y, r])).toBits()
+    let h = hashWithPrefix(
+      prefixes.signatureTestnet,


I chose to hard-code the signature to 'testnet', since the distinction between testnet and mainnet for signatures which aren't about transactions seemed not useful to me and just adds noise IMO

I think this is actually still very important if these signatures are going to be used in zkApps -- the same security vulnerability applies: If I sign something on the testnet, it could be replayed on mainnet and my funds can be drained if I use the same keypairs/nonce.

That's a good point! It opens a can of worms though. When signing transactions, the signature verifier is the Mina node, which has a well-defined network - it's either a mainnet node or a testnet node, so it knows which one it should use for verifying.
However, a smart contract currently doesn't have that. Do you think the network type should be hard-coded into the smart contract / circuit somehow? Should we leave it as a constant for the developer to define? (But then we're pushing to them the problem of switching this flag when deploying / compiling to either network).

I think, if we go this route, the network type should definitely be a constant in the circuit and not a Bool that the developer can make variable. Because if it can be variable, it's too easy a mistake to just make it a private user input which doesn't constrain the proofs to be valid for the respective network.

Hmm I suppose it’s much less dangerous than a transaction so I think it’s not worth the complexity cost of making these signatures distinct, but we should probably run this by more people. I’ll approve but I’m going to send a link to one of our slack channels and let’s let it bake overnight if anyone disagrees before we land it

am I understanding this right that the constant doesn't matter here because it's what the zkapp will verify and so you could pretty much use anything?

@mimoo yes, doesn't matter here in the sense that these signatures won't be sent to either network. we are completely in user-land: signatures are inputs to proofs created locally, where they are verified in-snark.

I think Brandon highlights an important problem: we don't provide replay protection for the user here, especially given that signatures are deterministic (nonce is derived from the message and private key). so, we currently leave it up to the application to figure out how to prevent that.

I think its too deep and broad a problem to solve in the context of this PR, but I created an issue so we don't forget to discuss this: #720

mitschabaude · 2023-01-31T13:36:53Z

src/lib/signature.ts

+    // TODO: Scalar.fromBits interprets the input as a "shifted scalar"
+    // therefore we have to unshift e before using it
+    let e = unshift(Scalar.fromBits(h.toBits()));


crypto change: So this seems like a bug in Scalar.fromBits, which is somewhat hard to resolve in that function directly, since this function would need to unshift the scalar in a SNARK-compatible way. It seemed easier to unshift out of the snark (here) and do the unshifting at the level of curve points when we're in the snark (see verify below)

mitschabaude · 2023-01-31T13:37:28Z

src/lib/signature.ts

+// performs scalar multiplication s*G assuming that instead of s, we got s' = 2s + 1 + 2^255
+// cost: 2x scale by constant, 1x scale by variable
+function scaleShifted(point: Group, shiftedScalar: Scalar) {
+  let oneHalfGroup = point.scale(Scalar.fromBigInt(oneHalf));
+  let shiftGroup = oneHalfGroup.scale(Scalar.fromBigInt(shift));
+  return oneHalfGroup.scale(shiftedScalar).sub(shiftGroup);


crypto change: this is how we undo the scalar shift inside the SNARK

why not just do

point.scale((shiftedScalar.sub(shift).mul(inv_2))

oh I think I see, the scalars are in the wrong field :D that sucks

yep exactly :D we don't have the foreign field arithmetic to do sub and mul on the scalar, inside the snark

mitschabaude · 2023-01-31T13:38:48Z

src/mina-signer/MinaSigner.ts

+  signFields(fields: bigint[], privateKey: Json.PrivateKey): Signed<bigint[]> {
+    let privateKey_ = PrivateKey.fromBase58(privateKey);
+    let signature = sign({ fields }, privateKey_, 'testnet');


crypto change: very straight-forward new function in mina-signer to sign an arbitrary list of field elements. it uses the existing sign and hard-codes the network to 'testnet' since this is not a transaction signature

would it makes sense to hardcode the chain id to 'zkapp' or something else then?

I think it does! Do you think we need actual domain separation between testnet signatures and signatures meant for zkapps? Or is it more about better naming, so that 'zkapp' is an alias for 'testnet'? (it wouldn't be hard to add the real domain separation fwiw)

sorry for late reply. In general I would say we should use domain separate as much as possible, because it's cheap/free : ) it's possible that it's not exploitable, but still why not distinguish it

mitschabaude · 2023-01-31T13:40:37Z

src/mina-signer/tests/verify-in-snark.unit-test.ts

@@ -0,0 +1,49 @@
+import { Field, isReady, shutdown } from '../../snarky.js';


this file tests that the new signatures are really compatible between mina-signer and snarkyjs, and also checks that they can be verified inside a SNARK

bkase · 2023-01-31T17:57:20Z

src/lib/signature.ts

-    const e = Scalar.fromBits(
-      Poseidon.hash(msg.concat([publicKey.x, publicKey.y, r])).toBits()
+    let h = hashWithPrefix(
+      prefixes.signatureTestnet,


I think this is actually still very important if these signatures are going to be used in zkApps -- the same security vulnerability applies: If I sign something on the testnet, it could be replayed on mainnet and my funds can be drained if I use the same keypairs/nonce.

bkase · 2023-01-31T18:01:11Z

src/snarky.d.ts

@@ -1274,7 +1279,7 @@ declare class Ledger {
  /**
   * Signs an account update.
   */
-  static signAccountUpdate(
+  static signOtherAccountUpdate(


why did this name change?

This is an unrelated fix, the name was actually signOtherAccountUpdate in the ocaml bindings, so it didn't work before

bkase

Product okay (tentatively, see comment). Still needs a crypto review from someone

mimoo

LGTM!

mimoo · 2023-02-09T01:05:38Z

src/lib/signature.ts

-    const e = Scalar.fromBits(
-      Poseidon.hash(msg.concat([publicKey.x, publicKey.y, r])).toBits()
+    let h = hashWithPrefix(
+      prefixes.signatureTestnet,


am I understanding this right that the constant doesn't matter here because it's what the zkapp will verify and so you could pretty much use anything?

mimoo · 2023-02-09T01:14:28Z

src/mina-signer/MinaSigner.ts

+  signFields(fields: bigint[], privateKey: Json.PrivateKey): Signed<bigint[]> {
+    let privateKey_ = PrivateKey.fromBase58(privateKey);
+    let signature = sign({ fields }, privateKey_, 'testnet');


would it makes sense to hardcode the chain id to 'zkapp' or something else then?

mimoo · 2023-02-09T01:17:49Z

src/lib/core.ts

@@ -72,3 +74,8 @@ That means it can't be called in a @method or similar environment, and there's n
    highBit: Bool(x >> lowBitSize === 1n),
  };
 };
+
+Scalar.fromBigInt = function (scalar: bigint) {


same question as elsewhere, I'm curious how a dev would know not to use these functions if they want them to create constraints? My guess is that we're warning them that going to bigint or from bigint is not constraining anything?

mimoo · 2023-02-09T01:26:47Z

src/lib/signature.ts

 }
+
+let shift = ScalarBigint(1n + 2n ** 255n);
+let oneHalf = ScalarBigint.inverse(2n)!;


can't you do let oneHalf = Scalar.fromBigInt(ScalarBigint.inverse(2n));?

no.. because Scalar.fromBigInt needs the JSOO / Wasm bundle to be loaded, and that's not the case at the top level. (only after await isReady)

mimoo · 2023-02-09T01:34:20Z

src/lib/signature.ts

+// performs scalar multiplication s*G assuming that instead of s, we got s' = 2s + 1 + 2^255
+// cost: 2x scale by constant, 1x scale by variable
+function scaleShifted(point: Group, shiftedScalar: Scalar) {
+  let oneHalfGroup = point.scale(Scalar.fromBigInt(oneHalf));
+  let shiftGroup = oneHalfGroup.scale(Scalar.fromBigInt(shift));
+  return oneHalfGroup.scale(shiftedScalar).sub(shiftGroup);


why not just do

point.scale((shiftedScalar.sub(shift).mul(inv_2))

mimoo · 2023-02-09T01:36:52Z

src/lib/signature.ts

-    let r = point.scale(e).neg().add(Group.generator.scale(this.s));
+    // TODO: Scalar.fromBits interprets the input as a "shifted scalar"
+    // therefore we have to use scaleShifted which is very inefficient
+    let e = Scalar.fromBits(h.toBits());


is this going to create constraints?

yeah sure! toBits does the proper in-snark unpacking of a field element, and fromBits is a no-op, since scalars are already represented as an array of bits (unfortunately, the bits represent a shifted scalar :/)

mimoo · 2023-02-09T01:39:37Z

src/lib/signature.ts

+function unshift(shiftedScalar: Scalar) {
+  return shiftedScalar
+    .sub(Scalar.fromBigInt(shift))
+    .mul(Scalar.fromBigInt(oneHalf));
 }


why does this only work out of snark? Also, isn't this a problem if I want to sign within a snark? (since it's used in the signature algo)

only works out of snark because sub and mul are not available on variable scalars. not sure how efficient they would be to implement in-snark, compared to the extra scaling I have to do instead. do you know?

the signing algorithm was already (I think, deliberately) written to not work inside a snark (for example, it used a constant random field element for the nonce 😱 ; now I'm deriving the nonce as a blake2b hash. tbf, the nonce could be a prover witness). the above will throw an error in the snark.

Inherently, signing in the snark is not as useful because then you have to provide your private key as input -- but that usually sits in some guarded place like your wallet. so, I think it's fine to only support signing outside + verifying inside which achieves the same and plays well with wallets

mitschabaude added 10 commits January 30, 2023 21:40

make in-snark signatures compatible

c38adac

sign fields in mina-signer

3ed2c24

test for compatible signatures (failing)

4b78b56

fix

c47db0f

add Scalar.fromBigInt

b240511

fix snarkyjs signatures which used shifted scalar

027da35

implement verification

4527eb6

finish unit test

a68a910

Merge branch 'main' into mina-signer/snark-compatible-signing

a27e68c

changelog

1249cc2

mitschabaude marked this pull request as ready for review January 31, 2023 13:30

mitschabaude commented Jan 31, 2023

View reviewed changes

bkase requested changes Jan 31, 2023

View reviewed changes

bkase approved these changes Jan 31, 2023

View reviewed changes

mimoo approved these changes Feb 9, 2023

View reviewed changes

mitschabaude merged commit 8109a64 into main Feb 10, 2023

mitschabaude deleted the mina-signer/snark-compatible-signing branch February 10, 2023 16:05

		@@ -0,0 +1,49 @@
		import { Field, isReady, shutdown } from '../../snarky.js';

Mina-signer: in-SNARK verifiable Signatures #710

Mina-signer: in-SNARK verifiable Signatures #710

Conversation

mitschabaude commented Jan 30, 2023 • edited Loading

mitschabaude left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitschabaude Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitschabaude Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitschabaude Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

mitschabaude Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimoo Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitschabaude Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkase left a comment

Choose a reason for hiding this comment

mimoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitschabaude Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

mitschabaude commented Jan 30, 2023 •

edited

Loading

mitschabaude Feb 9, 2023 •

edited

Loading

mitschabaude Jan 31, 2023 •

edited

Loading

mitschabaude Jan 31, 2023 •

edited

Loading

mitschabaude Jan 31, 2023 •

edited

Loading

mimoo Feb 9, 2023 •

edited

Loading

mitschabaude Jan 31, 2023 •

edited

Loading

mitschabaude Feb 9, 2023 •

edited

Loading